fix(panel): the conversation is always panel-owned — retire the workflow/ask chat scopes (mcp#884) - #680

Panel-side companion to artokun/comfyui-mcp#897 (MERGED 2026-08-06), which made agent sessions orchestrator-scoped — one session per backend across every panel, tab and workflow, keyed and persisted by the orchestrator. That half landed; this is the half that was still missing. Until it lands, main ships a panel that can still put the user in a per-workflow conversation while the orchestrator runs a single global session, so the transcripts silently diverge from the agent's real context.

What this changes

1. The workflow/ask chat scopes are retired. chatScopeMode() is hard-wired to "panel". The Settings → "Chat conversation scope" combo, panelHooks.applyChatScope, askModeFollowsPanel and the "ask" window.confirm are all removed — there is no live scope switcher left to reintroduce per-workflow sessions behind the orchestrator's back. A stored workflow/ask value from an older build is ignored, not migrated. Per-workflow threads created under the old modes stay in history and open through the picker like any archived conversation; nothing is deleted.

historyScopeFollowsPanel() is kept as a named seam (now constant) so the remaining per-workflow guards stay honest defense-in-depth rather than silently deleted invariants. That is a reasonable choice and also a standing hazard — the whole retired path wakes up the moment chatScopeMode() stops being a constant — so it is now pinned by gates rather than left to inspection (below).

2. Which conversation a tab renders is SHARED state, not tab-local. One selector — selectPanelThread — resolves it on cold restore and on cross-tab sync alike. A tab either acts (loadThread / newChat: dispatch the session frame, then publish the pointer) or adopts (reads the pointer and repaints). Adoption is passive, so N tabs never race to reset the one global session.

3. The selection pointer is BACKEND-scoped (panel:backend:<id>), the same axis as the orchestrator's orchestrator::<backend> session key — a Claude tab's selection must not move a Codex tab's conversation. resolvePanelPointer keeps a one-way legacy fallback to panel:global: once a backend key is written (including a deliberate clear) it never falls back again.

4. Stale-pointer guard, selection evidence only. Only SELECTION ops compete, total-ordered by compareRevisions. Message createdAt is deliberately not evidence — an imported archive, a straggler write or a skewed clock must not move the shared conversation. The retired workflow mode stamped a workflow-scoped active op on every thread open, so a genuine pre-upgrade snapshot restores the conversation the user was actually in.

5. Turn-output fencing. Ownership is pinned at user_message dispatch (pinTurnOwnerAtDispatch), not only at turn:working; onSay/onStream/onTodo return early and record() drops any non-user entry when the shown conversation is not the turn's owner. The straggler is dropped like an interrupt's, deliberately not re-routed into its owner thread (a fresh stamp there would yank the shared selection straight back).

6. The backend switch asks what the incoming backend HAS. A codex review found three P1s that shared one cause: the new per-backend conversation model was still coupled to the old provider-switch flow, which was built when switching always meant a fresh session. Fixed at two seams, not three point-fixes and not one (see Why two seams below):

• The upgrade fork (lib/chat-history-store.js). A pre-upgrade snapshot holds one shared panel:global pointer and no per-backend keys, so every backend key fell back to the same thread id: Claude and Codex both claimed it, loadThread scrubbed its foreign session, and record() rewrote its provider on every append — two providers sharing and corrupting one transcript, on the first upgrade of every existing user. The legacy route is now forked by provider, the ownership stamp record() already writes. Both doors are closed: the legacy pointer and the no-pointer recency fallback, which was equally "the same id for every backend". A pointer a backend wrote for itself still outranks the stamp, so normal operation is untouched (a thread legitimately changes provider when the user switches backends while it is open). A provider-less thread fails closed; nothing is deleted and it still opens through the picker.
• One handover decision (lib/backend-switch.js). planBackendHandover asks the store once what the incoming backend already has and derives both consequences, so they cannot drift apart again: (a) the outgoing thread keeps its sessionId — the tab pointer still goes — so switching back resumes instead of sending new_session, which is the per-backend persistence this PR exists to add and was being defeated by its own switch path; (b) the outgoing transcript is armed as one-shot context only when the incoming backend has no conversation. When it has one, loadThread resumes it and any armed context is cleared rather than merely skipped — anything armed earlier would otherwise ride the next user message into the wrong conversation.

Merge with main

This branch was ~440 commits behind. origin/main is merged in (now current through v0.14.41); 11 conflict hunks in comfyui-mcp-panel.js and 1 in workflow-chat-identity.spec.ts, resolved as:

• Reconciled with main's lib/interactive-card-fence.js. Main independently built an interactive-card fence for the same problem class — its header records that it was found by the independent gate on this PR and deliberately scoped to leave fix(panel): the conversation is always panel-owned — retire the workflow/ask chat scopes (mcp#884) #680's structural work alone. Both fences survive, because they are not interchangeable: fenceInteractiveCard() (main's classifier) guards ask_user/request_secret and weighs agentWorking + lastMintedThreadId as well, so an owner-less turn that minted its own thread is not mistaken for one painting into somebody else's conversation. This branch's turnOutputFenced() asks only "is the shown conversation the turn's owner", which is right for transcript output but would false-refuse that legitimate card — and a refusal costs the agent a tool error, so it must be the precise test.
• pinTurnOwnerAtDispatch narrows main's fence's own documented residual: a turn whose turn:working is discarded by the stale-working guard no longer reaches the classifier with a null owner.
• Main's bridgeOutage tracker (The mid-task nudge measures one backoff step, not the outage — a fast-returning restart loses its nudge #1145) supersedes this branch's lastBridgeDownAt; main's i18n tr() conversions kept throughout.
• interactive-card-fence.test.mjs's "record() does not write the turn owner" assertion is matched as an assignment rather than a bare substring. Reading the owner inside record() is legitimate and now happens (the output fence consults it); the substring form also failed on the comment explaining that fence — the prose-predicate trap, which would have been "fixed" by renaming a comment while leaving the real rule unguarded. The accepted assignment forms mirror the write-enumeration above it, so a liveTurnThreadId ||= thread.id smuggled into record() still fails.
• workflow-chat-identity.spec.ts: main's e2e: 2 specs fail — chat-history-v2 row count, workflow-chat-identity first record #847 save-then-assert-the-embed sequence is dropped. It passed only because the test forced the workflow scope this PR retires — the graph embed is written solely on the workflowStorageKey({ embed: true }) path, which panel scope never calls. Under the one shipping mode nothing is saved and nothing is embedded; provenance lives in history metadata and the graph is never touched.
• Removed the i18n catalog entries orphaned by the retired setting (comfyui-mcp_chatScope in settings.json plus 8 panel.* keys in main.json, across all 12 locales) — caught by the i18n round-trip and settings-key gates.

Why two seams and not one

The review suggested collapsing all three P1s into a single decision in the switch path. Two of them do collapse — session preservation and replay disposal are both consequences of "what does the incoming backend have", and they are now one function so a future edit cannot fix them by halves.

The upgrade fork cannot join them: cold restore reaches it with no switch involved. A reload on a pre-upgrade snapshot resolves the pointer directly through selectPanelThread, so a fix living in runBackendSwitch would leave the corruption fully reachable by simply reloading the tab. It is a resolution rule and belongs in the store. Forcing it into the switch path would have been a worse design that also failed to fix the bug.

Tests

npm run test:unit — 4449 tests, 4448 pass, 0 fail, 1 todo. npm run typecheck clean. check-panel-scope passes (every name resolves — the gate that catches a name resolving in a sibling function, the live risk when merge resolution moves code between functions).

One test, #671 verifyInstalled … inside the reply window, is a wall-clock budget assertion that fails only when the box is loaded (it failed once during a run that took 104 s against a normal 40-70 s, and passes 122/122 in isolation). It covers ComfyUI-Manager install verification and is untouched by this branch.

Complementary gate suites pin the invariants:

• browser_tests/unit/chat-history-store.test.mjs — source-level gates over the whole retired surface: chatScopeMode is a constant, no scope setting is registered, currentHistoryScopeKey resolves to a backend axis, every selection-pointer write uses the backend key, the workflow-keyed session bind is unreachable.
• browser_tests/unit/chat-scope-retired.test.mjs — the behavioural half of the retirement: it extracts the shipped chatScopeMode and calls it against an injected settings store, for every value a retired scope could have persisted.
• browser_tests/unit/turn-output-fence.test.mjs (new — closes the fence gap, see below).
• browser_tests/unit/backend-switch.test.mjs — the handover: planBackendHandover as a pure decision, the effects it drives through runBackendSwitch, and the shipped invalidateDurableAgentSession body extracted and run.
• browser_tests/unit/chat-history-store.test.mjs — the upgrade fork, including a test that two backends cannot resolve one legacy thread id, the provider-less fail-closed case, and the recency-fallback door.

Both behavioural suites are deliberately not source-regex tests. if (false) return getSetting(...) and if (false && turnOutputFenced()) match every regex anyone would write about those call sites while shipping the opposite behaviour, so only running them distinguishes the two.

Mutation results (committed first, then broken, then restored from an in-memory snapshot)

All twelve KILLED, re-run in full against the current head.

Mutations 3, 4 and 7 are the ones that mattered. 3 and 4 were this PR's headline behaviour with no CI coverage at all; 7 is the upgrade path every existing user takes.

Two of these survived their first run and are worth the reviewer's attention, because both are the failure mode where a fix looks tested and is not:

• [2] regressed. It was killed before the P1 fixes and survived afterwards: the new upgrade fork masked it. Its fixture threads carried no provider, so after the fork another backend's op resolved to a thread this backend could not claim anyway, and deleting the panel: skip stopped failing anything. The two rules are different questions — "could this backend own it" vs "is another backend's selection evidence for mine" — and the guard is now pinned with a thread whose provider satisfies both, which is reachable precisely because a thread's provider changes when the user switches backends while it is open.
• [9] was never covered. The handover tests proved runBackendSwitch passes preserveThreadSession; nothing proved the panel honours it. An invalidateDurableAgentSession that ignores the option destroys the outgoing session exactly as before and every assertion stayed green. The shipped body is now extracted and run, in both directions: a switch preserves the thread session, a restart still destroys it.

(Mutation 10 reports fewer total tests because it makes a whole test file throw at load; that is still a detection, just a louder one.)

The runner asserts an exact occurrence count for every anchor before applying it, and restores from an in-memory snapshot rather than git checkout. That guard earned its keep twice: mutation 1's first anchor matched 3 sites and was reported NOT APPLIED rather than as a kill.

How the turn-output fence gap (mutation 4) was closed

browser_tests/unit/turn-output-fence.test.mjs lifts the real turnOutputFenced, pinTurnOwnerAtDispatch, record, onSay, onStream and onTodo bodies out of the shipped panel source and executes them over stubbed collaborators — the established "real panel source" convention (interactive-card-fence.test.mjs's buildLifecycle(), context-ring-scope.test.mjs).

The earlier concern was that record() (124 lines) would need ~35 stubs and the harness would become a copy of the panel. It did not: the surface is 15 one-line stubs, every one either an observation point (persistThreads counts, appendAgent records what it was handed) or a fixture value (getWorkflowTitle, workflowTabId). The two collaborators that carry real logic — ChatHistoryStore (reviseThread/touchMessage) and isThreadInScope — are imported for real, so no thread-revision semantics are modelled in the test. Panel constants (MAX_THREADS, MAX_THREAD_MSGS, MAX_WORKFLOW_VERSIONS) are read out of the source rather than restated.

Extracting the fence predicate into web/js/lib/ was considered and rejected: turnOutputFenced() is already a correct two-line predicate, and testing it in isolation would not have killed this mutation. The mutation disables the call sites, not the predicate — if (false && correctPredicate()) passes any test written about correctPredicate. Only running the call sites separates them.

What the 14 new tests assert, all through the shipped bodies:

• an abandoned turn's agent record is dropped — not filed under the adopted conversation, and not re-routed into its owner thread either;
• a fenced record does not fall through to record()'s mint branch and create a conversation for output nobody asked for;
• a user entry is exempt and still lands in the view the user typed into (and is the same object — record() mutates in place, never clones);
• onSay / onStream / onTodo paint nothing when fenced;
• both halves agree — the painting fence and the recording fence read one predicate, so output can never paint without being recorded or vice versa;
• ownership really is armed at dispatch, before any turn:working frame arrives;
• and every normal path is unchanged.

Each source anchor is asserted to occur exactly once before it is evaluated. This checkout is CRLF, and an LF-authored multi-line anchor that silently misses would inject an empty body and make every assertion vacuous — which reads exactly like a passing test. (That guard earned its keep: the first run of mutation 1's own anchor matched 3 sites and was correctly reported as NOT APPLIED rather than as a kill.)

Not verified: Playwright

The Playwright specs are not in CI (they need a live ComfyUI on :8188 with --workers=1) and were not re-run against this head. The adoption, backend-switch and abandoned-turn e2e claims should be treated as unverified. The unit gates above now cover the fence, the scope retirement, the upgrade fork and the handover in CI, but the multi-tab adoption choreography and the live backend-switch round trip are still only claimed, not demonstrated, on this head — and the backend-switch changes above are exactly the area a live round trip would exercise.

Known residual (unchanged, and it is the honest one)

The panel publishes the shared selection off sendFrame() returning true, which proves only that the local socket accepted bytes — not that the orchestrator applied the transition. Two connected actors racing can therefore still disagree. Closing that needs orchestrator-confirmed session transitions, which do not exist on the wire; it is mcp-side work, and main's interactive-card-fence.js independently reached and documented the same conclusion.

🤖 Generated with Claude Code